Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement traits, impls and generic functions with trait bounds #710

Merged
merged 2 commits into from
Jun 16, 2022

Conversation

cburgdorf
Copy link
Collaborator

@cburgdorf cburgdorf commented May 18, 2022

Abstract

We want to have traits as well as generic functions with trait bounds. One of the main motivations is to properly implement emit on the Context struct as pub fn emit<T: Event>(val: T) { ... }

How was it fixed?

Working example below:

trait Computable {
fn compute(self, val: u256) -> u256;
}
struct Mac {}
impl Computable for Mac {
fn compute(self, val: u256) -> u256 {
return 1 + val
}
}
struct Linux {
pub counter: u256
pub fn get_counter(self) -> u256 {
return self.counter
}
pub fn something_static() -> u256 {
return 5
}
}
impl Computable for Linux {
fn compute(self, val: u256) -> u256 {
return val + Linux::something_static() + self.get_counter()
}
}
struct Runner {
pub fn run<T: Computable>(self, _ val: T) -> u256 {
return val.compute(val: 1000)
}
}
contract Example {
pub fn generic_compute(self) {
let runner: Runner = Runner();
assert runner.run(Mac()) == 1001
assert runner.run(Linux(counter: 10)) == 1015
}
}

Some notes:

  • Generic functions a currently monomorphized at MIR lowering
  • Because trait functions are usually (and currently only) defined without body the signature part was ripped out of FunctionId and into FunctionSigId to allow traits to reuse a bunch of code that doesn't care about the body anyway.
  • There are certain limitations that can potentially be improved in the future e.g.
    • support traits for non-struct types
    • allow trait methods to define a body as a default implementation
    • support generic functions in traits (e.g. trait PartialEq { pub fn eq<T>(self, other: T); })
    • support multiple bounds
    • support unbounded generics
    • support trait associated functions (e.g. SomeTrait::some_fun()

@cburgdorf cburgdorf force-pushed the christoph/feat/traits2 branch 9 times, most recently from a858bdb to a268c9a Compare June 3, 2022 07:03
@cburgdorf cburgdorf force-pushed the christoph/feat/traits2 branch 7 times, most recently from 93c68c1 to d0ba7c0 Compare June 8, 2022 08:50
@cburgdorf cburgdorf changed the title Exploring Traits Implement traits, impls and generic functions with trait bounds Jun 8, 2022
@cburgdorf cburgdorf marked this pull request as ready for review June 8, 2022 09:09
@cburgdorf cburgdorf marked this pull request as draft June 13, 2022 19:51
@cburgdorf
Copy link
Collaborator Author

I just changed this back to draft because I will update it soon™ with all the remaining parts to cover impl blocks. Just need a few more cleanups and bits here and there.

@cburgdorf cburgdorf force-pushed the christoph/feat/traits2 branch 5 times, most recently from 588483d to 48525b2 Compare June 14, 2022 08:41
@cburgdorf cburgdorf force-pushed the christoph/feat/traits2 branch 4 times, most recently from 91c3387 to f97afea Compare June 14, 2022 10:28
@cburgdorf cburgdorf marked this pull request as ready for review June 14, 2022 10:37
Copy link
Member

@Y-Nak Y-Nak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left some comments.

Also, the below code causes ICE.

trait Computable {
  fn compute(self) -> u256;
}

struct Mac {
  pub fn compute(self) -> u256 {
     return 0
  }
}

impl Computable for Mac {
  fn compute(self) -> u256 {
    return 1
  }
}

contract Bar {
  pub fn foo() -> u256 {
    let mac: Mac = Mac()
    return mac.compute()
  }
  
  pub fn foo2() -> u256 {
    let mac: Mac = Mac()
    return foo_computable(val: mac)
  }
}

fn foo_computable<T: Computable>(val: T) -> u256 {
  return val.compute()
}
thread 'main' panicked at 'should be lowered in `lower_types_in_functions`', crates/mir/src/lower/types.rs:25:13
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

I haven't looked into this yet, but it'd be better to address this in this PR.

crates/analyzer/src/db/queries/impls.rs Outdated Show resolved Hide resolved
@@ -747,6 +772,7 @@ impl ModuleConstantId {
pub enum TypeDef {
Alias(TypeAliasId),
Struct(StructId),
Trait(TraitId),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to discuss whether traits should be regarded as a type. For our use case, it feels be better to treat trait as an Item.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, yes. That's a good point. I initially made it a type because I started by ignoring generics to just support something like:

pub fn bar(val: SomeTrait) {
}

But then later I noticed that it would be better to add generics first and only support

pub fn bar<T: SomeTrait(val: T) {
}

I don't know if we ever want to support the first example (or maybe val: impl SomeTrait as rust does) or just leave it to generics entirely. My current thinking is that we should not support the first example and leave it to generics to work with traits and hence could remove Trait as a type and add it as its own category of Item.

crates/analyzer/src/namespace/items.rs Show resolved Hide resolved
crates/analyzer/src/namespace/items.rs Outdated Show resolved Hide resolved
crates/analyzer/src/namespace/items.rs Show resolved Hide resolved
crates/analyzer/src/namespace/items.rs Show resolved Hide resolved
crates/mir/src/lower/function.rs Outdated Show resolved Hide resolved
@cburgdorf
Copy link
Collaborator Author

cburgdorf commented Jun 15, 2022

Also, the below code causes ICE.

Thanks, this code should have been rejected because generics should not be supported yet outside of struct functions. Fixed it on my local machine.

@Y-Nak
Copy link
Member

Y-Nak commented Jun 15, 2022

Ok, I misunderstood the cause of ICE. I wrote the test case to check what would happen when method name conflicts happen, and it caused ICE, so I wrongly suspected that function name conflict was the cause of the ICE.

After modifying the test case, it causes ICE because of name conflicts this time.

trait Computable1 {
    fn compute(self) -> u256;
}

trait Computable2 {
    fn compute(self) -> u256;
}


struct Mac {
}

impl Computable1 for Mac {
    fn compute(self) -> u256 {
        return 1
    }
}

impl Computable2 for Mac {
    fn compute(self) -> u256 {
        return 2
    }
}

contract Bar {
    pub fn foo() -> u256 {
        let mac: Mac = Mac()
        let mys: MyStruct = MyStruct()
        return mys.foo_computable1(val: mac)
    }
    
    pub fn foo2() -> u256 {
        let mac: Mac = Mac()
        let mys: MyStruct = MyStruct()
        return mys.foo_computable2(val: mac)
    }
}

struct MyStruct {
    pub fn foo_computable1<T: Computable1>(self, val: T) -> u256 {
        return val.compute()
    }

    pub fn foo_computable2<T: Computable2>(self, val: T) -> u256 {
        return val.compute()
    }
}

Error: DeclarationError: Function name foo$Mac$compute already taken in this scope.
  --> input.yul:51:13:
   |
51 |             function foo$Mac$compute() -> $$ret {
   |             ^ (Relevant source part starts here and spans across multiple lines).


thread 'main' panicked at 'Yul compilation failed with the above errors', crates/driver/src/lib.rs:192:13
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

You've hit an internal compiler error. This is a bug in the Fe compiler.
Fe is still under heavy development, and isn't yet ready for production use.

If you would, please report this bug at the following URL:
  https://github.com/ethereum/fe/issues/new

So we need to mangle the function name to fix the bug, I think it would be better to implement the mangling in codegen, not in mir.

@cburgdorf
Copy link
Collaborator Author

Oh, I see. I believe this is because here we are considering the name of the class which for the impl is the class of the receiver. If instead we would have the impl here instead then we could consider a combination of trait name and receiver name which should make it unique.

if let Some(class) = analyzer_func.class(db.upcast()) {
let class_name = class.name(db.upcast());
format!("{}::{}{}", class_name, func_name, type_suffix).into()

think it would be better to implement the mangling in codegen, not in mir

Do you suggest to fix it here?

pub fn symbol_name(db: &dyn CodegenDb, function: FunctionId) -> Rc<String> {
let module = function.signature(db.upcast()).module_id;
let module_name = module.name(db.upcast());
let func_name = function.name_with_class(db.upcast()).replace("::", "$");
format!("{}${}", module_name, func_name).into()

Wouldn't that mean that we let name_with_class return an faulty name? Why wouldn't we want to fix it earlier?

Follow up question, we could fix that by making Impl a variant of Class but you suggested to get rid of Class altogether. If we do that should Impl also become an Item and should we consider trait name + receiver name as the name of the impl?

@Y-Nak
Copy link
Member

Y-Nak commented Jun 15, 2022

Oh, I see. I believe this is because here we are considering the name of the class which for the impl is the class of the receiver.

Partially yes, this is one of the concrete examples. But I didn't come up with a concrete example when I reviewed it, I just felt that it may reduce too much of the amount of information that should be kept.

Wouldn't that mean that we let name_with_class return an faulty name? Why wouldn't we want to fix it earlier?

Yes, it would work perfectly in this case. I just thought that name mangling should be performed in the codegen phase because the mangled name is never used from another compilation pass. But yes, fixing the class perfectly works in this case.

If we do that should Impl also become an Item and should we consider trait name + receiver name as the name of the impl?

I think the "name" of the function would differ depending on the context. What context is in your head?

Currently, the "name" of the function is used in two contexts,

  1. Printing MIR for debugging,
  2. symbol_name in codegen
    For 1., the name would be <MyStruct as MyTrait>::method_name like Rust's mir.
    For 2., the name would be MyTrait::MyStruct::method_name or even method_name_{impl_id} would be sufficient.

Now, I'm feeling that defining separate two functions for the two purposes and removing name_with_class would be the best option, instead of tweaking name_with_class.

@cburgdorf cburgdorf force-pushed the christoph/feat/traits2 branch 2 times, most recently from 5ac761f to 468e585 Compare June 16, 2022 09:01
@cburgdorf
Copy link
Collaborator Author

Thanks again for the feedback @Y-Nak
I renamed name_with_class to debug_name and made it format the impl function as <MyStruct as MyTrait>::method_name per your suggestion. I also tweaked symbol_name directly so that MyTrait$MyStruct$method_name is used there.

I didn't feel like adding the removal of Class to this PR so instead I added Class::Impl for now as an interim solution.

I think the only thing left from your suggestions is the removal of Trait from TypeDef which I started with but then ran into issues. Since I'm not yet entirely sure if we eventually want to support using traits in places for types maybe we can leave that as is for now and revisit later as we extend and tune the trait support?

Copy link
Member

@Y-Nak Y-Nak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work @cburgdorf!

we can leave that as is for now and revisit later as we extend and tune the trait support?

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants